virttools: add new test type and virt-install test #3923

smitterl · 2021-12-09T11:15:25Z

Depends on avocado-vt PR avocado-framework/avocado-vt#3270

/virttools:
Add new test type for tools from virt-manager repository; by
using avocado-vt we get access to many useful test functions
ready to use for qemu/libvirt testing. The test type assumes
the same default setup as for tp-libvirt/libvirt and builds
on top of that. During testing, switching from a bootstrapped
libvirt environment to a bootstrapped virttools environment
worked and the test case passed.
hostdev_mdev:
Add a new test case.
a. 'run': This virt-install test case is currently ccw device
specific but other types on other platforms can be
implemented by creating a subclass for the
MdevHandler for the desired mdev_type.
b. 'clean_up/sleep': The introduction of 1sec sleep is
necessary to make sure setting the device offline will
work. Using the usual 'wait_for' here seemed to
overcomplicate the code.
c. 'virt_install_with_hostdev': The function hardcodes several
vm installation parameters. This is to keep the code
simple at this point but can be changed in the future.
spell.ignore:
Add command and device names used in ccw.py and hostdev_mdev.py.
ccw.py:
Add a new function to set devices offline to reset them to their
previous state. Test cases with real ccw devices usually assume
the test environment is configured in a way that the first unused
ccw device will be used for the test case, possibly in a
destructive way. Some functions have been added return value 'True'
on success to allow for usage in 'wait_for' constructions.
README.md:
Add readme file to give a bit more context and intention for the
new test type.

Signed-off-by: Sebastian Mitterle [email protected]

smitterl · 2021-12-09T11:16:02Z

Test run

# avocado run --vt-type virttools --vt-machine-type s390-virtio --vt-connect-uri qemu:///system virt_install.hostdev.mdev
JOB ID     : 9a063022d47f0f96f4cc438878e3fdaba8db872d
JOB LOG    : /root/avocado/job-results/job-2021-12-09T06.12-9a06302/job.log
 (1/1) type_specific.io-github-autotest-libvirt.virt_install.hostdev.mdev.check_present_inside_guest: PASS (17.59 s)
RESULTS    : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 17.91 s

smitterl · 2021-12-09T11:20:10Z

Regression coverage for changes in ccw.py:

# avocado run --vt-type libvirt --vt-machine-type s390-virtio --vt-connect-uri qemu:///system libvirt_ccw_passthrough
JOB ID     : be82f5b0b4906dcc2857bec16dfeb0e554e27f48
JOB LOG    : /root/avocado/job-results/job-2021-12-09T06.17-be82f5b/job.log
 (1/3) type_specific.io-github-autotest-libvirt.libvirt_ccw_passthrough.read_write.happy_path: PASS (49.78 s)
 (2/3) type_specific.io-github-autotest-libvirt.libvirt_ccw_passthrough.happy_path: PASS (33.68 s)
 (3/3) type_specific.io-github-autotest-libvirt.libvirt_ccw_passthrough.device_removal: PASS (50.45 s)
RESULTS    : PASS 3 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 134.35 s

smitterl · 2021-12-09T11:44:38Z

@dzhengfy @chunfuwen @Yingshun Please, can you help review?

dzhengfy

Others LGTM

dzhengfy · 2021-12-17T01:17:55Z

virttools/tests/src/virt_install/hostdev_mdev.py

+        raise NotImplementedError()
+
+    @staticmethod
+    def from_type(mdev_type):


I think the function name is not very meaningful and explicit. Could we use one similar like 'get_instance_from_type()'?

@dzhengfy I would like to maintain it, IMO, it's a common python idiom, please check https://github.com/avocado-framework/avocado-vt/pull/2610/files#r445400984

dzhengfy · 2021-12-17T01:25:30Z

virttools/tests/src/virt_install/hostdev_mdev.py

+            ccw.set_device_offline(self.device_id)
+
+
+def disk_for_import(vmxml):


To align the naming style for functions in your codes, I suggest we name this function beginning with a verb, like 'get_disk_for_import()'. But the decision depends on you.

I updated it.

dzhengfy · 2021-12-17T01:26:02Z

virttools/tests/src/virt_install/hostdev_mdev.py

+    return first_disk.find('source').get('file')
+
+
+def first_mdev_nodedev_name():


get_first_mdev_nodedev_name()? But this decision depends on you

I updated it.

dzhengfy · 2021-12-17T01:26:50Z

virttools/tests/src/virt_install/hostdev_mdev.py

+
+        session = vm.wait_for_login()
+        handler.check_device_present_inside_guest(session)
+


It is better to close the session.

The session is closed now in the final clause handler.clean_up.

Yingshun · 2021-12-17T02:34:15Z

virttools/tests/src/virt_install/hostdev_mdev.py

+            raise TestError("Test doesn't know how to handle %s." % mdev_type)
+
+
+class CcwMdevHandler(MdevHandler):


How about moving CcwMdevHandler and MdevHandler to provider? I wonder if they can be used by other tests...

@Yingshun At this point the base class doesn't do anything but only serves as interface to be implemented. Especially the get_target_address method at this point to me looks rather test specific. I'm not sure if somebody found those classes below the provider path they would know what to do with them without the context of the test implementation. Therefore, I'd like to wait for other tests to need these classes before moving them. Is that okay?

Yingshun · 2021-12-17T02:38:00Z

virttools/tests/src/virt_install/hostdev_mdev.py

+        :raises: TestFail if device not found
+        """
+        device, _ = ccw.get_first_device_identifiers(self.chpids, session)
+        if not device == self.expected_device_address:


I would suggest using inline negation, please refer to https://www.python.org/dev/peps/pep-0008/#programming-recommendations(Use is not operator rather than not ... is. While both expressions are functionally identical, the former is more readable and preferred:...).

Yingshun · 2021-12-17T02:47:51Z

virttools/tests/src/virt_install/hostdev_mdev.py

+
+        virt_install_with_hostdev(vm_name, mdev_nodedev, target_address, disk)
+
+        session = vm.wait_for_login()


Do you need to start the vm before trying to loin?

Do you need to start the vm before trying to loin?

No, the virt-install command boots the guest.

chunfuwen · 2022-01-17T09:32:02Z

virttools/tests/src/virt_install/hostdev_mdev.py

+from virttest import virsh
+
+
+class MdevHandler(object):


Just curious that in terms of MdevHandler, would you like maintain it as local test module in hostdev_mdev.py , or it could be more generic module shared across test modules?

@chunfuwen I think your question is covered in discussion above #3923 (comment)

Yingshun · 2022-01-18T02:22:55Z

virttools/tests/src/virt_install/hostdev_mdev.py

@@ -0,0 +1,198 @@
+import logging


Please add 'LOG = log.getLogger('avocado.' + name)' and replace logging.xxx with LOG.xxx in the code. The current way does not log messages in the latest avocado. For details, please check #3833

smitterl · 2022-01-21T16:19:50Z

Rerun after requested changes:

JOB ID     : f58d52f08ef6cf45371d843f36aee267ed7e2f8c
JOB LOG    : /root/avocado/job-results/job-2022-01-21T11.15-f58d52f/job.log
 (1/1) type_specific.io-github-autotest-libvirt.virt_install.hostdev.mdev.check_present_inside_guest: PASS (18.27 s)                                                                         
RESULTS    : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
JOB TIME   : 18.59 s

chunfuwen

LGTM

Yingshun

Others LGTM

Yingshun · 2022-01-24T06:45:01Z

provider/vfio/ccw.py

+
+    :param device_id: cssid.ssid.devno, e.g. 0.0.560a
+    :param session: guest session, command is run on host if None
+    :raises TestError: if the device can't be set online


Should 'online' be 'offline'?

No, there's already a function set_device_online

Sorry @smitterl ,I should speak full sentence to make the comment clearer.
This function is set_device_offline() which used to set the device offline. And from the code, we can see that it'll raise TestError if the device can't be set offline, but not online. So I mean, I think it's a typo here, and it should be like:

Suggested change

:raises TestError: if the device can't be set online

:raises TestError: if the device can't be set offline

Got it now @Yingshun You are right. I fixed it. Thank you!

1. /virttools: Add new test type for tools from virt-manager repository; by using avocado-vt we get access to many useful test functions ready to use for qemu/libvirt testing. The test type assumes the same default setup as for tp-libvirt/libvirt and builds on top of that. During testing, switching from a bootstrapped libvirt environment to a bootstrapped virttools environment worked and the test case passed. 2. hostdev_mdev: Add a new test case. a. 'run': This virt-install test case is currently ccw device specific but other types on other platforms can be implemented by creating a subclass for the MdevHandler for the desired mdev_type. b. 'clean_up/sleep': The introduction of 1sec sleep is necessary to make sure setting the device offline will work. Using the usual 'wait_for' here seemed to overcomplicate the code. c. 'virt_install_with_hostdev': The function hardcodes several vm installation parameters. This is to keep the code simple at this point but can be changed in the future. 3. spell.ignore: Add command and device names used in ccw.py and hostdev_mdev.py. 4. ccw.py: Add a new function to set devices offline to reset them to their previous state. Test cases with real ccw devices usually assume the test environment is configured in a way that the first unused ccw device will be used for the test case, possibly in a destructive way. Some functions have been added return value 'True' on success to allow for usage in 'wait_for' constructions. 5. README.md: Add readme file to give a bit more context and intention for the new test type. Signed-off-by: Sebastian Mitterle <[email protected]>

Yingshun

LGTM

chunfuwen · 2022-01-28T10:15:08Z

comments had been addressed, let's merge it

smitterl force-pushed the virt_tools_single_commit branch from f881c2a to 06e1994 Compare December 9, 2021 11:21

This was referenced Dec 9, 2021

backends: add new test type virttools avocado-framework/avocado-vt#3270

Merged

virttools: add type and first test #3865

Closed

smitterl force-pushed the virt_tools_single_commit branch 2 times, most recently from 31031c7 to 73a563f Compare December 9, 2021 17:29

dzhengfy added the depend on The PR has dependency on other PRs label Dec 16, 2021

dzhengfy reviewed Dec 17, 2021

View reviewed changes

Yingshun reviewed Dec 17, 2021

View reviewed changes

smitterl force-pushed the virt_tools_single_commit branch 2 times, most recently from 8249f0d to dfa4178 Compare January 4, 2022 16:49

chunfuwen reviewed Jan 17, 2022

View reviewed changes

Yingshun reviewed Jan 18, 2022

View reviewed changes

smitterl force-pushed the virt_tools_single_commit branch from dfa4178 to 0dcd75d Compare January 21, 2022 16:18

smitterl force-pushed the virt_tools_single_commit branch from 0dcd75d to 3d05e37 Compare January 21, 2022 16:22

smitterl requested review from Yingshun, chunfuwen and dzhengfy January 21, 2022 16:23

chunfuwen approved these changes Jan 24, 2022

View reviewed changes

Yingshun reviewed Jan 24, 2022

View reviewed changes

smitterl requested a review from Yingshun January 27, 2022 15:14

smitterl force-pushed the virt_tools_single_commit branch from 3d05e37 to 3532df3 Compare January 28, 2022 09:31

Yingshun approved these changes Jan 28, 2022

View reviewed changes

chunfuwen merged commit da8c84f into autotest:master Jan 28, 2022

smitterl deleted the virt_tools_single_commit branch July 28, 2022 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

virttools: add new test type and virt-install test #3923

virttools: add new test type and virt-install test #3923

smitterl commented Dec 9, 2021

smitterl commented Dec 9, 2021

smitterl commented Dec 9, 2021

smitterl commented Dec 9, 2021

dzhengfy left a comment

dzhengfy Dec 17, 2021

smitterl Dec 30, 2021

dzhengfy Dec 17, 2021

smitterl Jan 21, 2022

dzhengfy Dec 17, 2021

smitterl Jan 21, 2022

dzhengfy Dec 17, 2021

smitterl Jan 21, 2022

Yingshun Dec 17, 2021

smitterl Dec 30, 2021

Yingshun Dec 17, 2021

smitterl Jan 21, 2022

Yingshun Dec 17, 2021

smitterl Jan 4, 2022 •

edited

Loading

chunfuwen Jan 17, 2022

smitterl Jan 18, 2022

Yingshun Jan 18, 2022

smitterl Jan 21, 2022

smitterl commented Jan 21, 2022

chunfuwen left a comment

Yingshun left a comment

Yingshun Jan 24, 2022

smitterl Jan 27, 2022

Yingshun Jan 28, 2022

smitterl Jan 28, 2022

Yingshun left a comment

chunfuwen commented Jan 28, 2022

		ccw.set_device_offline(self.device_id)


		def disk_for_import(vmxml):

		return first_disk.find('source').get('file')


		def first_mdev_nodedev_name():


		session = vm.wait_for_login()
		handler.check_device_present_inside_guest(session)

		raise TestError("Test doesn't know how to handle %s." % mdev_type)


		class CcwMdevHandler(MdevHandler):


		virt_install_with_hostdev(vm_name, mdev_nodedev, target_address, disk)

		session = vm.wait_for_login()

	:raises TestError: if the device can't be set online
	:raises TestError: if the device can't be set offline

virttools: add new test type and virt-install test #3923

virttools: add new test type and virt-install test #3923

Conversation

smitterl commented Dec 9, 2021

smitterl commented Dec 9, 2021

smitterl commented Dec 9, 2021

smitterl commented Dec 9, 2021

dzhengfy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitterl Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitterl commented Jan 21, 2022

chunfuwen left a comment

Choose a reason for hiding this comment

Yingshun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yingshun left a comment

Choose a reason for hiding this comment

chunfuwen commented Jan 28, 2022

smitterl Jan 4, 2022 •

edited

Loading